Skip to content

Conversation

@claudiu-cristea
Copy link
Member

We are able to alter/replace commands in modules but we cannot intercept and alter the command info (annotations). This PR adds such capability.

How it works?

  • Modules wanting to alter the command info should add a service class that implements \Consolidation\AnnotatedCommand\CommandInfoAltererInterface.
  • The service should be added in drush.services.yml and should be tagged with the drush.command_info_alterer tag.
  • Add the alter logic in the alterCommandInfo() of the alterer class.

See the testing woot module for details.

@claudiu-cristea claudiu-cristea changed the title Allow to register command info alterer services Allow registering of command info alterer services Mar 7, 2018
@weitzman
Copy link
Member

weitzman commented Mar 7, 2018

Woot woot! Lets see what @greg-1-anderson has to say.

@greg-1-anderson
Copy link
Member

The code looks good and I am generally +1 on this. Could you be a little more specific about what you are doing with this? Are you just renaming / re-aliasing existing commands, as the tests are doing?

$serviceCommandInfoAltererlist = $container->get(DrushServiceModifier::DRUSH_COMMAND_INFO_ALTERER_SERVICES);
$commandFactory = Drush::commandFactory();
foreach ($serviceCommandInfoAltererlist->getCommandList() as $altererHandler) {
$commandFactory->addCommandInfoAlterer($altererHandler);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably write a debug log message if a module adds one of these.

@greg-1-anderson
Copy link
Member

My big concern here is that folks could get really confused if one module is altering commands in core or another module that is not related (in the same suite as) to the module doing the alteration.

Could you write some documentation on how this feature could be used, and also strongly encourage folks to debug log when altering commands, so that this behavior is discoverable to anyone who does get confused. It won't be possible for us to enforce logging, of course, but we can at least encourage it.

It would also be good if you could inject a logger into the woot command and debug log there, just so that the test code serves as a good example of how to alter commands. It isn't necessary to have an assert that ensures the log is printed, as long as it's there for illustrative purposes.

@claudiu-cristea
Copy link
Member Author

@greg-1-anderson, we are preparing to add migrate commands to Drush core in #3402. But we want to coordinate with migrate_tools/migrate_plus so that migrate_plus-specific migrations will still work. We will achieve this by removing the Drush commands from migrate_tools and adding command alters/replacements in migrate_plus. Now, commands are easy to replace, using hooks. For example, in migrate_plus we swap the Drush core migrate status command in this way: @hook replace-command migrate:status. But also the annotations need some changes. For example, on the same command, we have different output structure in Drush core command than in migrate_plus. The @field-labels annotation need to be swapped. So, migrate_plus module has to add some alters to some command info, too.

@greg-1-anderson
Copy link
Member

Perhaps some of these operations should happen automatically in annotated command. I haven't checked yet to see if this is possible / easy, but if other annotations in a @hook command-replace always completely overwrote the annotations in the hooked command, would that reduce the number of things you need to alter? Would you ever find it undesirable if @hook command-replace overwrote overlapping annotations?

@claudiu-cristea
Copy link
Member Author

claudiu-cristea commented Mar 7, 2018

@greg-1-anderson , I saw that array annotations, such as @option, @Usage are merged, not replaced. The others are kept from the original command.


class WootCommandInfoAlterer implements CommandInfoAltererInterface, LoggerAwareInterface
{
use LoggerAwareTrait;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can drop the LoggerAwareInterface / LoggerAwareTrait here. Drush will only inject its logger into command classes that implement these interfaces -- not any object at all. Since you are instantiating this class via the Drupal service file (drush.services.yml), then Drupal will inject your logger factory, so the code you have below is sufficient.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, removed the trait and interface.

Copy link
Member

@greg-1-anderson greg-1-anderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@greg-1-anderson
Copy link
Member

LGTM thanks for contributing this - should be very useful.

@greg-1-anderson greg-1-anderson merged commit 98aabe6 into drush-ops:master Mar 7, 2018
@claudiu-cristea claudiu-cristea deleted the command-info-alter branch March 7, 2018 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants